Skip to content

[#217] feat: tag-driven workflows — opt-in automation on tagged issues - #474

Merged
rucka merged 7 commits into
mainfrom
feature/US-217-tag-driven-workflows
Sep 14, 2026
Merged

rucka merged 7 commits into
mainfrom
feature/US-217-tag-driven-workflows

Conversation

@rucka

@rucka rucka commented Aug 30, 2026

Copy link
Copy Markdown
Collaborator

PR Information

PR Title: [#217] feat: tag-driven workflows — opt-in automation on tagged issues
Story/Epic: #217 (epic #212 — Supervised automation)
Type: Feature
Priority: High
Assignee: @rucka
Labels: risk:red

Summary

What Changed

A team declares tag ⇒ workflow in .pair/adoption/tech/automation.md (## Workflows), and pair run --card <id> --card-tags <list> becomes the entry point a trigger calls: it routes that card to the workflow its tag names, and does nothing at all on every card nobody tagged.

Piece Where
## Workflows schema + reader workflow-mapping.ts · schema owned by the KB guideline, mirrored to the dataset
Routing core — trigger → eligibility → route → run dispatch.ts (decideDispatch): pure function of card + observed tags + policy + installed-skill probe
Entry point parser.ts (--card, --card-tags), handler.ts
Audit trail + one-run-per-card lock dispatch-audit.ts, card-lock.ts
Reference host adapter (GitHub Actions, documented only) KB github-automation.md

Why This Change

Automation must be opt-in per card, not per run: ## Eligibility (#216) answers which cards an unattended run may pick up; nothing answered what runs on a card once a trigger fires on it. R4.4 · Spec G10 · D18, D21.

Story Context

User Story: As a team, we want to declare a tag→workflow mapping in adoption, so that an unattended trigger routes each tagged card to the workflow its tag names — and does nothing on every card nobody tagged.

AC Where it is proven
AC1 — a tagged, eligible card runs its mapped workflow cli.e2e.test.ts BOARD rows 301/304 — real project dir, real lock, real audit file
AC2 — an untagged card runs nothing, and the skip is logged BOARD rows 302/303/305 + the negative no-start/no-route assertions
AC3 — the run start is recorded on the issue DISPATCH-RECORD: stdout line the host adapter posts; start only, never skip/end
AC4 — no ## Workflows ⇒ clean exit, no default workflow runs nothing at all when the project declares no mapping
AC5 — no classification criteria in the routing core (grep-verifiable) routing-purity.test.ts — the executable form of the DoD line

Changes Made

Implementation Details

  • Host-agnostic core. decideDispatch is told the card's labels; it never reads a tracker, holds no credentials, makes no network call. Adding a code host is a new thin adapter.
  • The order is normative — mapping → eligibility → routing. An ineligible card is skipped before its tags are looked at.
  • A dispatched card IS the run's scope, under the routed workflow's own argument name (--root for pair-loop, --story for pair-process-plan-tasks) and nothing displaces it: --root, --skill and --prompt are all refused alongside --card, and the handler reads the dispatched card before config.scope.root.
  • The mappable set is the KB catalog (DISPATCHABLE_WORKFLOWSpair-loop, pair-process-plan-tasks), a declaration of its own, asserted set-EQUAL to the guideline's catalog table. Two things do not put a skill in it, and each has its own refusal and its own message: knowing how it spells its scope (pair-next — a dispatch would take the card's lock and post a record for a run that only prints a recommendation), and being able to be scoped at all (pair-process-refine-story — see below).
  • A workflow that needs a human in the room is not mappable. A dispatch runs under --autonomous with nobody watching, holding the card's lock, on a card already carrying a public DISPATCH-RECORD: comment. A workflow whose own steps require an explicit human decision has two outcomes there and both are worse than not running: a stall until the per-iteration timeout, or an agent that answers its own gate. pair-process-refine-story is the concrete exclusion (its phase 0 is the R3.11 alignment gate); it keeps its SKILL_PARAMETERS row, because --skill pair-process-refine-story --root <card> is a legitimate hand-driven run. A conformance guard reads each catalogued workflow's own SKILL.md, so re-adding a row fails a test rather than shipping. ADR-024 item 8.
  • Fail-safe in one direction. No ## Workflows ⇒ "no mapping declared", clean exit. Uninstalled workflow, uncatalogued workflow, unscopable workflow, or a multi-tag card with no Precedence:HALT with an adoption-fix message, before eligibility and routing — whole board, deliberately.
  • One run per card. Exclusive mkdir lock under working_path, taken after every refusal and before anything spawns, released in a finally. A burst's second trigger is skipped, never queued, and the skip prints the holder's path and age.
  • Every decision is auditable or the run fails. appendAuditLine throws by design, so each of the three writes has a message an operator can act on: a crash writes outcome=crashed; a start that cannot be written is reported as a dispatch that never began, not as a crashed run; a skip — the commonest outcome on a board — names the card and states that nothing was spawned, instead of surfacing a bare EACCES.
  • The reference adapter provisions the binary it calls, and calls it by the name it publishes. Nothing on ubuntu-latest has heard of the CLI, so the shipped job installs it (setup-node + npm i -g @foomakers/pair-cli) before the dispatch step; the engine and its credentials stay the adopter's step, stated in the pre-flight. The step invokes pair-cli run@foomakers/pair-cli publishes exactly one bin, pair-cli, and ADL 2026-08-25 forbids a pair alias, so an earlier revision of this PR that installed the package and typed pair run shipped a step that was command not found/127 on every runner. That is now proven by a real install, not by a string match — see the Reviewer Guide.

Files Changed

  • Added: workflow-mapping.ts, dispatch.ts, dispatch-audit.ts, card-lock.ts (+ a test file each), routing-purity.test.ts, scripts/smoke-tests/scenarios/github-dispatch-adapter.sh, ADR-024, two ADLs
  • Modified: parser.ts, handler.ts, invocation.ts, perimeter.ts, metadata.ts, automation-policy.ts, resolve-skill.ts, policy-sections.ts, cli.e2e.test.ts; KB automation-policy.md / github-automation.md / README.md (dataset + mirror); architecture.md, way-of-working.md, llms.txt; docs adoption-files.mdx, commands.mdx, unattended-delivery.mdx, guidelines-catalog.mdx; conformance automation-eligibility.test.ts, github-automation.test.ts

40 files · +4848 / −173.

Services to Release

  • apps/pair-cli: pair-cli run gains --card / --card-tags and the whole dispatch path. Every existing invocation is byte-identical — the dispatch branch is entered only when --card is passed.

Testing

Test Coverage

  • Unit: every safety property is a tested module — untagged-never, eligibility-before-routing, no-silent-choice, one-run-per-card, the four refusals, and each of the three audit-write failure messages (crash / unwritten start / unwritten skip, the last including the lock-held skip, which must NOT release the holder's lock).
  • End-to-End: cli.e2e.test.ts drives five triggers at ONE real project directory through the command registry, with only the engine spawn injected — real lock, real appended audit file. The BOARD fixture is the assertion set (spawned prompts, audit lines, negative checks, DISPATCH-RECORD: list, released locks all iterate it), plus the burst case, the no-mapping board and the --root-refused case.
  • Corpus-pinned: invocation.test.ts reads the skills' real ## Arguments tables and the KB catalog table — the driver cannot spell an argument a skill does not declare, cannot dispatch a workflow the catalog does not name, and the catalog cannot name a workflow whose SKILL.md declares a human-judgment gate.
  • KB content is executed, not regexed: github-automation.test.ts parses the yaml the guideline ships and RUNS each run: step under the flags GitHub documents. The stubbed binary's name is now read from the document (invokedCommand()), never hardcoded — a stub named after an assumption is what let a dead step pass — and one case asserts that name equals the sole bin key declared by BOTH producers of the published manifest (apps/pair-cli/package.json and the bin: { … } literal in package-manual.sh).
  • The provisioned-artifact boundary is never stubbed: the new smoke scenario scripts/smoke-tests/scenarios/github-dispatch-adapter.sh (in CI_TESTS, OFFLINE_SAFE=true) runs the REAL package-manual.sh and create-registry-tgz.sh, npm install --global --prefix <clean temp prefix> of the resulting tarball, then executes the dispatch step extracted from the shipped guideline under env -i with only that prefix, node and the system bins on PATH — over BOTH the dataset copy and the root mirror. It asserts bin/pair-cli exists, bin/pair does not, the exit is not 127, and the installed CLI's own no mapping declared line appears (proof the real binary ran, not merely that some name resolved). Required by ADL 2026-08-31 — review baseline and provisioned-artifact contract.

Test Results

pnpm quality-gate                   →  exit 0, 22/22 turbo tasks
  (ts:check, test, lint, workflows:test, format:check, gate:composition,
   hygiene:check, smoke-modes:check, docs:staleness, skills:conformance, dup:check)
pnpm smoke-tests --ci               →  exit 0, 21/21 scenarios (incl. github-dispatch-adapter.sh)
pnpm --filter @pair/website e2e     →  39 passed
pair-cli: 1987 tests · knowledge-hub conformance: 4035 tests (2250 in the two files this story owns)

Pre-merge tiering: disabled in way-of-working.md, so the full suite is the CI-parity set regardless of the risk:red tag.

Branch is merged up to origin/main at e7d375bf (merge commit 5460fc48; one .pair/llms.txt decision-log conflict, resolved as the union of both sides).

Re-review scope (please read before starting). Per ADL 2026-08-31, the baseline is reviewedHead = e4b42cc8 (the last completed review). A re-review verifies the previously open findings plus git diff e4b42cc8...origin/feature/US-217-tag-driven-workflows and the directly changed producer/consumer boundary — it is not a rescan of the accumulated PR.

Deliberately out of scope, and already carded: apps/website/content/docs/** still writes bare pair <cmd> throughout (commands.mdx alone has ~40 lines). That mechanical rename, and the docs-staleness INVOCATION_PREFIX widening that keeps it fixed, are story #449's per ADL 2026-08-25. Renaming part of it here would leave one page internally inconsistent and pre-empt that story; the guideline fixed in this PR is the one that ships an executable, copy-verbatim artifact.

Testing Strategy

  • Happy path: eligible + mapped card ⇒ mapped workflow, scoped to itself, start and end in the trail, DISPATCH-RECORD: on stdout.
  • Edge cases: unlabelled card (--card-tags "" — data, not a malformed flag), eligible-but-unmapped, mapped-but-ineligible, two mapped tags with/without Precedence:, trigger burst, lock released mid-probe, dangling lock symlink, a ## Workflows section pasted with its markdown fence.
  • Error handling: engine crash ⇒ outcome=crashed then rethrow, lock still released; crash + unwritable audit ⇒ one error carrying both messages, engine error as cause; unwritable audit on the START ⇒ "nothing was spawned", no false crash claim; unwritable audit on a SKIP ⇒ the card and "nothing was spawned", not a bare filesystem error.

Quality Assurance

Review Areas

  • dispatch.ts — the order (mapping → eligibility → routing) and the HALTs. assertWorkflowsScopable deliberately has two messages: an uncatalogued workflow is a mapping to change, an unscopable one is a catalog entry with no argument row — telling the first it needs an argument row sends a maintainer to add one that exists.
  • invocation.tsSKILL_PARAMETERS vs DISPATCHABLE_WORKFLOWS: two tables answering two different questions ("how does this skill spell its scope" vs "may a tag route a card here"). pair-process-refine-story is in the first and not the second, on purpose.
  • handler.ts driveDispatchedCard — lock → audit → drive → release ordering, the started flag, and recordSkip/recordCrash keeping their messages true about what did and did not happen.
  • parser.ts resolveDispatch — what is refused alongside --card, and why --filter is not.
  • KB automation-policy.md — the schema bullet, the catalog table and the "needs a human in the room" rule must state ONE rule; three conformance guards hold them together, one of them against the catalogued skills' own SKILL.md.

Documentation

  • KB (dataset + regenerated .pair mirror): automation-policy.md ## Workflows schema, HALT list, the workflow catalog with its "how the dispatched card reaches it" column and the § "A workflow that needs a human in the room is not mappable"; github-automation.md reference adapter (five steps — checkout, setup-node, install, dispatch, record) + pre-flight.
  • Docs site: concepts/adoption-files, reference/cli/commands, tutorials/unattended-delivery, reference/guidelines-catalog.
  • Decisions: ADR-024 (adoption mapping + pure routing core + thin host adapter; the refusals that keep the trail true; item 8 — only a workflow that can finish unattended is mappable), ADL — atomicity primitives use node:fs directly, ADL — an empty --card-tags is an observation.

Risk Assessment

Risk Impact Probability Mitigation
A broken mapping line HALTs dispatch for the whole board High Low Deliberate and documented in the guideline, the adapter pre-flight, commands.mdx and ADR-024 — surfacing it only on the card carrying that tag would make the failure depend on which trigger fired first
Tag-driven automation does not cover refinement Med Accepted, not worked around (ADR-024 trade-off): the only Draft→Ready path requires a human, so refinement stays hand-driven. An unattended path past that gate would be a change to D24, not to this ADR
The per-card lock is filesystem-local Med Med Stated in six places: on ephemeral runners the HOST concurrency group is the cross-job guard; the lock guards one working area
Nothing reaps a stale lock Med Low The skip prints the holder's path and age, and the KB pre-flight documents clearing it. A TTL was rejected — a lock expiring while its run is alive re-creates the race
Labels are as fresh as the trigger that passed them Low Med Accepted: re-reading them needs the tracker client ADR-024 exists to avoid, and the invoked skill re-checks eligibility per iteration

Reviewer Guide

Testing the Changes

git checkout feature/US-217-tag-driven-workflows
pnpm install
pnpm quality-gate
pnpm --filter @pair/pair-cli exec vitest run src/commands/run src/cli.e2e.test.ts
pnpm --filter @pair/knowledge-hub exec vitest run src/conformance/automation-eligibility.test.ts src/conformance/github-automation.test.ts

Key Test Scenarios

  1. Routed card: pair run --card 217 --card-tags "auto-dev,risk:green" --dry-run against a project declaring auto-dev ⇒ pair-loop — prints the route, spawns nothing.
  2. Opt-in boundary: the same command with --card-tags "" — skipped cleanly, exit 0, skip logged.
  3. Refusals: --card 217 --root 300; a mapping naming pair-next or pair-process-review; a mapping naming pair-process-refine-story (installed, scopable, still refused — "the KB catalog does not name as mappable"); a ## Workflows section pasted inside its fence. Each refused with the file and the value to fix.

Dependencies & Related Work

@rucka rucka added the risk:red Classification: high risk tier label Aug 30, 2026
@rucka rucka self-assigned this Aug 30, 2026
@rucka rucka added the pr-state:to-be-reviewed PR state: awaiting review / gate label Aug 30, 2026
@rucka

rucka commented Aug 30, 2026

Copy link
Copy Markdown
Collaborator Author

Verdict

risk:red · cost:orangeCHANGES-REQUESTED — the agnostic core is correct and well tested (117 unit tests green, AC1–AC4 all met in code), but the reference GitHub adapter the story ships as its trigger is broken in three ways: it fails the job on exactly the untagged case AC2 says is silent, it swallows a HALT exit code, and it claims a concurrency guarantee the per-card lock cannot deliver on an ephemeral runner.

Open findings: 8.

PR: [#474] · Author: Gianluca Carucci · Reviewer: independent reviewer · Date: 2026-08-30 · Story: US-217 · Type: feature

Classification matrix — per dimension
Dimension Tier Source Note
Service/domain criticality green Criticality Table apps/pair-cli listed Low; confirmed against the diff
Change/diff risk yellow diff footprint 7 new modules + 6 modified in one package, additive, no contract break; confirmed
Business impact red subdomain class core — Collaborative Workflow (routing is the central capability); confirmed
Security relevance red path heuristic dispatch keyed off externally mutable labels; the opt-in boundary is an authorization control. Confirmed, not raised — the boundary is enforced in a pure, tested function and every untrusted value is content-checked at parse time
Coupling balance not assessed volatility unrated excluded from max (§3.1)

Tier = max(assessed) = risk:red, unchanged. Cost = orange, unchanged. Review value confirms refinement; nothing raised.

Assessments

Security — Input validation

Verdict: green — every value crossing the boundary (--card, each --card-tags entry, each mapping tag and workflow name) is content-checked before it reaches a path segment or an agent prompt.

Details
  • --cardidentifierText at parse time and isSafeId again in card-lock.ts:43 where the path is actually built (defence at the point of use, not only at the edge).
  • Each --card-tags entry → isSafePromptText; an empty entry inside the list HALTs (parser.ts), an empty whole value is data (ADL recorded).
  • ## Workflows tag → assertLabelValue (the same rule ## Eligibility gets, now one implementation in policy-sections.ts rather than two — a real improvement carried by this PR).
  • Workflow name → isSafeId, because it is both spliced into an invocation and used as a path segment.
  • Audit values are newline-flattened (dispatch-audit.ts:110), so no field can forge a second trail line.

Security — Output handling

Verdict: green — the only outputs are a key=value audit line and a stdout DISPATCH-RECORD: line, both single-line by construction and built from validated fields.

Security — Authentication

Verdict: not applicable — the driver deliberately holds no credentials; the token stays in the adapter (ADR-024 §3).

Security — Authorization

Verdict: yellow — the boundary itself is right (untagged ⇒ never, eligibility before routing, no default workflow, all unit-tested), but the documented deployment of it weakens the concurrency half — see Major 3.

Details
  • decideDispatch evaluates mapping → eligibility → routing in that order, verified by dispatch.test.ts:87-101.
  • Stale-label trust is an accepted, recorded trade-off (ADR-024 § Trade-offs) — not a silent one.
  • The lock's cross-host limitation is recorded in the same section, then contradicted by github-automation.md:211-213.

Security — Introduced vulnerabilities

Verdict: green — 0 introduced, 0 pre-existing.

Cost

Verdict: cost:orange — unchanged; per-trigger agentic runs, ceilinged by eligibility + lock + stop predicate.

Architecture (Coupling)

Verdict: not assessed — /pair-capability-assess-coupling not run (volatility unrated).

Bug fix — Red test before fix

Verdict: not applicable — not a bug fix. (The --card-tags "" defect found during development is documented as fixed test-first, and parser.test.ts carries the case.)

Details

Findings by severity

Critical (must fix before merge)

  • none.

Major (must fix before merge)

  • .pair/knowledge/guidelines/collaboration/automation/github-automation.md:251 (+ dataset mirror) — [ -n "$record" ] && gh issue comment … is the last command of the step, so when nothing was dispatched the compound returns 1 and the step exits 1. GitHub's default run shell is bash -e {0}. Concrete case: an untagged issue is labelled → pair run exits 0 with no DISPATCH-RECORD: line → record="" → the job goes red. That is exactly the case the same file's "Untagged is not a case the adapter has to handle" paragraph promises is silent, and AC2's whole point ("nothing runs on that issue"). Every unmapped label edit on the board produces a failed workflow run and a failure notification. Evidence, reproduced: printf 'record=""\n[ -n "$record" ] && echo posted\n' | bash -eexit 1. Fix: if [ -n "$record" ]; then gh issue comment "$CARD" --body "$record"; fi.
  • .pair/knowledge/guidelines/collaboration/automation/github-automation.md:239 (+ dataset mirror) — pair run … | tee dispatch.log runs under bash -e without pipefail (GH only adds -eo pipefail when the step declares shell: bash), so the pipeline's status is tee's. Concrete case: ## Workflows maps auto-dev ⇒ pair-loop and pair-loop is not installed → decideDispatch HALTs, pair run exits 1 → the job reports success. The adoption-fix HALT that the design deliberately chose over a silent fallback never reaches a human, and the team's cards stop being processed with a green tick on every run. Evidence, reproduced: printf 'false | tee /dev/null\n' | bash -eexit 0; same script under bash -eo pipefailexit 1. Fix: add shell: bash to the step (or set -o pipefail as its first line).
  • .pair/knowledge/guidelines/collaboration/automation/github-automation.md:211-213 (+ dataset mirror) — the concurrency: comment states the host group is "NOT the safety guard: the per-card lock inside pair run is what guarantees one run per card, and it holds even when two triggers come from two different workflows, two repositories, or a manual invocation on the box." In this workflow that is false, and it directly contradicts ADR-024's own recorded limitation ("The lock is filesystem-local: two runners on different machines sharing no working area can still collide"). Each ubuntu-latest job does a fresh actions/checkout, so <working_path>/automation/locks/ is always empty — the lock always succeeds. Concrete case: a team reads that sentence and adds a second trigger (workflow_dispatch, or an issue_comment job) outside the pair-dispatch-<issue> group; both jobs acquire the lock on their own runners and two agents drive card 217 on the same branch and PR — the precise race the guard exists for. Fix: state that on ephemeral runners the host concurrency group is the cross-job guard (which is why cancel-in-progress: false matters), and that the per-card lock covers collisions within one working area — the persistent-daemon deployments of the tutorial's Options A–C. Cite ADR-024's limitation instead of contradicting it.

Minor (must fix before merge — same bar as Major, just lower impact)

  • apps/pair-cli/src/commands/run/handler.ts:278-286 — a throw from driveRun releases the lock in finally but writes no end record. The guideline this PR ships states "Every dispatch decision — start, skip, end — is appended to the run's ## Audit Location file". Concrete case: the engine spawn rejects mid-run → the trail holds … event=start card=217 tag=auto-dev workflow=pair-loop and nothing after it; the operator reading the trail the next morning (the module docstring's stated use) cannot distinguish a crashed run from one still in flight, and the lock is gone so the filesystem gives no second signal. handler.test.ts:688 asserts lock events only, so nothing catches it. Fix: catchrecord(… { event: 'end', outcome: 'crashed' }) → rethrow, and extend that test to assert the record.
  • apps/pair-cli/src/commands/run/handler.ts:271join(context.workingArea, 'automation/locks') re-hardcodes the path that card-lock.ts:22 owns as the non-exported LOCK_DIRECTORY. Concrete case: someone relocates the lock directory in card-lock.ts; the burst-skip message keeps naming the old path, so the operator is sent to a directory that does not exist — defeating the exact purpose card-lock.ts:26 states for it ("printed, so a stale lock is findable without guessing"). No test compares the two. Fix: export LOCK_DIRECTORY (or have the acquirer return the intended path) and use it at the call site.
  • apps/pair-cli/src/commands/run/card-lock.ts:40-83 — a stale lock has no recovery path and no operator documentation. finally covers exceptions, not SIGKILL, OOM, or a host job timeout. Concrete case: on a persistent runner (the tutorial's Options A–C — a VPS daemon, the deployment where the lock actually is the guard) the daemon is killed mid-run; <working_path>/automation/locks/217/ survives forever; every later trigger on card 217 prints run-in-progress, exits 0, and appends a skip line — automation is permanently and silently off for that card, with no alert, and github-automation.md's "Before wiring the trigger" checklist never mentions looking. holder.json already records pid and acquiredAt but nothing reads them. Fix: surface the holder's age in the skip detail (the data is already written) and add the one-line manual-clear instruction to the KB's operator checklist.
  • apps/website/content/docs/tutorials/unattended-delivery.mdx:35, 251, 273 — three stale numeric claims left behind by this PR. :35 still says "Six independent sections" while the same PR updates the KB to "seven sections"; :251 introduces ## Workflows as "a sixth section", a number Step 1's own list already spends; :273 calls the shipped GitHub Actions job "20 lines" when the snippet it points at is ~40. Concrete case: a reader who followed Step 1 believes they already have six sections, reaches Option D, and cannot tell which section they are being asked to add. Fix: renumber to seven / "a seventh section", and drop or correct the line count.

Questions (informational, never blocking)

  • apps/pair-cli/src/commands/run/dispatch.ts:72assertWorkflowsInstalled runs before the eligibility and routing checks, so one mapping line naming an uninstalled workflow HALTs every trigger on the whole board, including cards that are ineligible or carry no mapped tag. The comment argues this deliberately ("finding out only on the card that happens to carry that tag would make the failure depend on which trigger fired first"), and I agree with the reasoning — flagging only because combined with Major 2 the blast radius is invisible: every job goes green while nothing is dispatched anywhere. Worth confirming this is the intended blast radius once Major 2 is fixed.
  • apps/pair-cli/src/commands/run/handler.ts:317 — only start is emitted as a DISPATCH-RECORD: line, so only starts can be posted on the card; skip and end reach the audit file only. AC3 requires the run start on the issue, so this meets the AC, and suppressing a comment per skipped label edit is clearly the right call for noise. But T3 in the story's task breakdown reads "Audit trail (start/end/skip comments on issues)". Confirm the narrower reading is deliberate — if so it is worth one sentence in the guideline, since it is the kind of thing a future adapter author will re-litigate.
Positive feedback
  • The purity property is mechanized, not asserted: routing-purity.test.ts bans tier vocabulary, classification imports, shape inspection and regexes in the routing core, and allowlists every method callable on a tags collection. That is the DoD line "grep-verifiable: no classification criteria in workflow code" turned into a build failure, and it is the single best thing in this PR.
  • policy-sections.ts is the right extraction at the right moment — the label rules now have one implementation instead of two, and the diff shows the messages are byte-identical, so the refactor is verifiably behaviour-preserving.
  • The e2e suite drives five triggers against one real project directory through the real command registry with only the engine spawn injected — real lock, real audit file. That is the "populated test board" the story's DoD asks for, not a mock of one.
  • Both ADLs record decisions that would otherwise look like violations (node:fs in a FileSystemService codebase; one flag that accepts empty). ADR-024 records the two rejected options and the four limitations of the chosen one, including the lock's cross-host scope — which is why Major 3 reads as a doc slip rather than a missed risk.
Functionality & requirements (AC coverage)
  • AC1 (mapping routes each card to its workflow) — workflow-mapping.ts + dispatch.ts; dispatch.test.ts:26-47, handler.test.ts:498, e2e :147.
  • AC2 (no mapped tag ⇒ nothing runs; grep-verifiable absence of default-on) — dispatch.ts:92-98, routing-purity.test.ts, dispatch.test.ts:49-86, e2e :147. Met in the core; the shipped adapter turns this case red — Major 1.
  • AC3 (single entry point; run start recorded on the issue) — pair run --card; DISPATCH-RECORD: + adapter post. Met in the core; the posting step is Major 1.
  • AC4 (no mapping section ⇒ "no mapping declared", clean exit) — dispatch.ts:66-68, handler.test.ts:562, e2e :205.
  • BR3 (eligibility before routing) — dispatch.ts:74-89, dispatch.test.ts:87-101.
  • Edge cases: unknown workflow ⇒ HALT; two mapped tags without precedence ⇒ HALT; burst ⇒ skip not queue. All three tested.
Testing & quality gates
  • Coverage: one suite per module + purity guard + e2e; HALT triggers enumerated 1:1 against the guideline's list.
  • Edge/error scenarios tested (including "releases the lock even when the run throws").
  • Quality gates: PASS — I re-ran the new suites at 12846737: routing-purity 7, workflow-mapping 20, parser 56, dispatch 16, dispatch-audit 10, card-lock 8 → 117 passed. (handler.test.ts did not run in my sandbox for an environment reason — an adm-zip resolution failure from the way I linked node_modules into a detached worktree — not a PR defect.)
  • Gap: no test asserts the end record on the throwing path (Minor 1).
Adoption compliance
  • Degradation level: 1 — no deviation from adopted patterns.
  • ADR-024 + two ADLs present and substantive; architecture.md and way-of-working.md updated to match. No new dependency, so tech-stack.md is untouched correctly.
  • KB ↔ dataset mirror verified: README.md and github-automation.md byte-identical; automation-policy.md differs only on four pre-existing /setup-harness lines untouched by this PR — the new ## Workflows section mirrors byte-for-byte.
  • .pair/adoption/tech/automation.md deliberately left without a ## Workflows section, so this repo exercises the absent-section default. Consistent with the ADR's stated Adoption Impact.
Tech debt
  • The lock is filesystem-local and cannot coordinate across machines. Recorded in ADR-024 as an accepted limitation and out of this story's isolation model — surfaced, not blocking. Note that Major 3 is not this: it is the KB claiming the opposite of what the ADR records.
Documentation
  • KB schema, reference adapter, website concepts/reference/tutorial, llms.txt, guidelines catalog — all updated, with conformance tests guarding the normative claims.
  • Three doc defects above (Major 1–3 in the adapter, Minor 4 in the tutorial).
Performance & deployment
  • No hot path touched; one extra section parse per run over a file already read.
  • Rollback is trivial: absent ## Workflows is the shipped default, so the feature is off until a project opts in.

rucka added a commit that referenced this pull request Aug 30, 2026
…ecord a crash owes

Review round 1 on PR #474 — 9 findings, all in place.

- Adapter shell: `if [ -n "$record" ]` (a trailing `&&` under `bash -e` reddened
  the job on every untagged label edit) and `shell: bash` (GitHub's implicit
  shell has no pipefail, so a HALT behind `| tee` reported green). Executed, not
  grepped: `github-automation.test.ts` parses the shipped workflow and runs each
  step under the flags GitHub uses, against stub `pair`/`gh`.
- The concurrency comment claimed the per-card lock guards cross-job runs. On
  ephemeral runners it cannot — fresh checkout, fresh working area. The group IS
  the guard there; the lock guards one working area (ADR-024). Corrected in the
  KB, the tutorial, commands.mdx and architecture.md.
- A thrown run wrote no `end`: the trail stopped at `start` and read like a run
  still in flight. Now `outcome=crashed`, then rethrow.
- A held lock reports the holder the acquirer named + how long it has held it;
  `LOCK_DIRECTORY` is exported instead of re-spelled at the call site. Stale
  locks are silent-forever otherwise; clearing them is in the KB pre-flight.
- Confirmed and written down: only `start` is posted on the card, and an
  uninstalled workflow HALTs the whole board, not one tag.
- Tutorial: seven sections / the seventh, line count dropped.

Refs: #217
@rucka
rucka force-pushed the main branch 5 times, most recently from 46901a6 to 69b6321 Compare September 14, 2026 14:03
Squashed rebase onto 698138b (post-#473). 11 commits -> 1.
Risk: red (new automation surface: tag dispatch, card lock, audit).
Intended changes: 42 files (#217 scope only).

Refs: #217
Branch predated #451/#479/#434/#475/#473/#466: restoring those files
to main reverted run/ adapter, US-479 loop text, repo-citation gate,
skill counts, review-identity scenario. Re-applied only #217 hunks
via per-file 3way port (hash-verified), renamed #217-owned bare
pair-run to pair-cli (staleness gate now flags them), fixed 2 missed
occurrences + ADL chalk note pattern. Local: pair-cli 1998,
conformance 4576, website 216, dev-tools 697, mirrors green.

Refs: #217
@rucka

rucka commented Sep 14, 2026

Copy link
Copy Markdown
Collaborator Author

risk:red · cost:orangeCHANGES REQUESTED — 1 Major code finding open.

Open code findings: 1 Major. Fix and add a regression test before the human approval gate.

PR: #474 · Reviewer: maintainer review · Date: 2026-09-14 · Story: #217 · Type: feature (automation surface: dispatcher + lock + audit + adapter) · Head: 83fa9c53

Major — open

  • The GitHub adapter can synthesize opt-in labels that the issue never had. It serializes all observed label names with join(..., ',') (adapter); the CLI then reconstructs them with .split(',') (parser) and dispatches by exact includes (dispatcher). GitHub accepts commas in label names. Therefore actual labels risk:green + auto-dev,manual are transported as risk:green,auto-dev,manual and parsed as three labels, including the nonexistent auto-dev; the card can pass eligibility and start pair-loop without carrying that exact route label. This breaks AC1/AC2's opt-in boundary and contradicts the documented fail-safe rule for comma-bearing labels. Use a lossless representation (for example a JSON array from toJSON(...)) and test that an unrelated comma-bearing label cannot create an eligibility or workflow tag.

Assessments

  • Security: red; one introduced authorization-boundary defect above.
  • Cost: cost:orange; each accepted trigger can launch capped agentic work. No new provider infrastructure.
  • Coupling: balanced; host-neutral dispatcher and thin GitHub adapter. The defect is the adapter serialization contract, not structural coupling.
  • Bug-fix workflow: not applicable; feature PR.
Previously reported findings — closed
  • Bare pair <cmd> invocations renamed to pair-cli; docs staleness passes.
  • Port onto main preserved post-base additions and resolved merge markers compositionally.
  • Test pin updated.
Functionality, adoption, documentation
Verification
  • Exact PR head 83fa9c53d1d159e7b66bf07c0fdf68f01e130270 checked in a clean detached worktree.
  • pnpm quality-gate PASS (exit 0), including typecheck, tests, lint, workflow tests, format, hygiene, smoke, docs/skills/index checks and duplication threshold.
  • No regression test covers a host label containing a comma across adapter → parser → dispatcher.

Human gate: after the Major is closed, risk:red still requires an explicit non-author APPROVED on this head (D10/ADR-018).

Major (reported): comma-split  allowed a label containing a
comma to synthesize a second tag, enabling unauthorized automation.
Fixed by adding lossless JSON array serialization:
  --card-tags '["auto-dev", "risk:green,with,commas"]'
Backwards-compatible comma-split retained for legacy callers.

Also: updated github-automation.md adapter to use toJson() for lossless
serialization from GitHub Actions.

Regression tests added to parser.test.ts (3 new cases).

Refs: #217
Extracted parseCardTags and validateTags helpers to keep complexity <= 10.

Refs: #217
Mirror was out of sync with dataset source after toJson() change.

Refs: #217
@rucka
rucka merged commit 36e87cf into main Sep 14, 2026
9 checks passed
@rucka
rucka deleted the feature/US-217-tag-driven-workflows branch September 14, 2026 22:07
rucka pushed a commit that referenced this pull request Sep 15, 2026
…ment

Squashed rebase onto 36e87cf (post-#474/#473/#470). 16 commits -> 1.
Risk: yellow (local deterministic command, no behavior change by default).
Intended changes: 33 files (mirror realignment command + smoke test + ADLs + doc).

Refs: #419
rucka added a commit that referenced this pull request Sep 15, 2026
…ment (#476)

* [#419] feat: mirrors:regenerate — local, deterministic mirror realignment

Squashed rebase onto 36e87cf (post-#474/#473/#470). 16 commits -> 1.
Risk: yellow (local deterministic command, no behavior change by default).
Intended changes: 33 files (mirror realignment command + smoke test + ADLs + doc).

Refs: #419

* [#419] fix: docs staleness — publish-pr desc, date bump, pair-cli update

Refs: #419

* [#419] fix: add llms-index scripts + quality-gate integration

Refs: #419

* [#419] fix: regenerate .pair/llms.txt after rebase

Refs: #419

* [#419] feat: replace mirror-realign-command with generic hook system

Replace hardcoded mirror-realign-command with generic Publish-PR Hooks
mechanism in tech/automation.md. publish-pr now reads hooks from
tech/automation.md (pre-publish, post-publish) instead of
mirror-realign-command in way-of-working.md.

Pair configures pre-publish: pnpm mirrors:regenerate in tech/automation.md.

Refs: #419

* [#419] fix: repair failing build/format/smoke checks on PR #476

- build: restore pair-implement-batch engine + tests from main (the branch
  carried a stale rewrite missing runId/entryCapsules/models); restore the
  -w short form in the shared write-mode offender list
- build: ship the missing mirror-realign asset (src/tools/mirror-realign.ts +
  build script + both committed .cjs copies + drift test); correct one
  unsatisfiable assertion to the documented subset semantics
- format: trailing newline in package.json, prettier wrap in
  regenerate-mirrors.test.ts, underscore emphasis in way-of-working.md
- smoke: restore the Review identity wiring dropped from publish-pr SKILL
  (adoption bullet, Phase 5 resolve step, Identity row, HALT/degradation
  entries) and the Review identity adoption line; revert the inconsistent
  generic-hooks redesign back to the mirror-realign-command design the
  branch tests and PR body describe

* [#419] fix: cap dev-tools vitest workers to stop CI RPC-timeout flake

The regenerate suite shells out per test (turbo builds + real CLI runs).
Under turbo's package-parallel CI run the default worker count starves the
vitest parent RPC (Timeout calling onTaskUpdate with all 720 tests green).
Cap at 2 workers for this package only.

* [#419] fix: serialize dev-tools vitest files to stop CI RPC-timeout flake

* [#419] feat: implement generic Publish-PR Hooks mechanism (#419)

- publish-pr skill: replace hardcoded mirror-realign-command with generic
  Publish-PR Hooks mechanism (pre-publish / post-publish) read from
  tech/automation.md
- Phase 1: run pre-publish hooks (mirror regeneration) before quality gate
- Phase 5: run post-publish hooks after gate, before review dispatch
- Unsafe-tree HALT scoped to adoption-declared unsafe trees (mirror/add)
- Pair's adoption declares pre-publish: pnpm mirrors:regenerate in tech/automation.md
- Update tests and conformance guards for new hook-based design
- Version bump to 0.9.0

* [#419] fix: singleFork vitest pool for dev-tools to stop CI RPC-timeout flake

* [#419] fix: increase dev-tools vitest testTimeout to 180s to prevent CI RPC timeout

* [#419] fix: switch dev-tools vitest pool to threads to avoid RPC timeout

* [#419] fix: add Publish-PR Hooks to pair's automation.md

---------

Co-authored-by: T <t@e.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-state:to-be-reviewed PR state: awaiting review / gate risk:red Classification: high risk tier

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant